[Go] Generate type aliases for deduplicated inline model schemas#22694
[Go] Generate type aliases for deduplicated inline model schemas#22694AriehSchneier wants to merge 11 commits intoOpenAPITools:masterfrom
Conversation
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenProperty.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/CodegenProperty.java:195">
P2: `dataTypeAlias` was added but omitted from equals/hashCode/toString, so properties differing only by alias compare equal and hash the same</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@cubic-dev-ai please re-run a review. |
@AriehSchneier I have started the AI code review. It will take a few minutes to complete. |
@AriehSchneier I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java:1002">
P2: Alias vendor extension can be overwritten; deduplication alias is lost if source schema already defines x-alias-name.</violation>
<violation number="2" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java:1002">
P2: Alias metadata is added only for dedup via makeSchemaInComponents; other schema reuse paths still drop x-alias-name, so deduplicated inline schemas may miss alias information.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 7 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java:1002">
P2: Deduplicated inline schema alias ignores inlineSchemaNameMapping by storing the unmapped name in x-alias-name</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/InlineModelResolver.java:670">
P2: x-alias-name for deduplicated inline schemas bypasses inlineSchemaNameMapping, producing unmapped alias names</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
@wing328 I noticed you had kicked off the tests (which showed a failure), I fixed the test, any chance you can kick them off for me again? |
|
@AriehSchneier thanks for the PR if i'm not mistaken, the issue has to do with the option shouldn't we fix the option instead? |
|
@wing328 I think this should be done as well as the fix for I wanted to turn on In short this PR is not a fix for that bug, it is an enhancement which means I won't be using that feature and therefore will not personally be hitting that bug. (I hope that makes sense?) |
|
if we fix |
I would prefer this change over a fix for the above bug. Also note, this change would be useful for any language that has alias types, if you are more familiar with another language I could try to add that language in this PR. |
@wing328 Maybe I got something completely wrong, but |
You are correct, that wasn't actually a bug, I have closed that issue and removed the link from this PR. However I would still like this enhancement to proceed. |
Based on review feedback: 1. Improved template comment to be more specific about "deduplicated inline model schemas" 2. Added configuration option 'generateTypeAliasesForDedupedSchemas' (defaults to true) - Added constant to CodegenConstants - Added boolean field to AbstractGoCodegen - Added CLI option to GoClientCodegen - Made type alias generation conditional on this flag 3. Added documentation to Go generator docs explaining the new option The feature can now be disabled by setting: --additional-properties=generateTypeAliasesForDedupedSchemas=false Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Update: Configuration Option AddedTo help get this merged, I've added a configuration option Default behavior: This provides backward compatibility while enabling the improved naming context by default. Users who encounter any issues or prefer the old behavior can easily disable the feature without waiting for a new release. The implementation is complete with:
This should address any concerns about backward compatibility while still providing the benefits of contextual type names for the majority of users. |
Added the new configuration option to the test suite: - Added GENERATE_TYPE_ALIASES_FOR_DEDUPED_SCHEMAS_VALUE constant - Added option to createOptions() map - Added verification in GoClientOptionsTest This fixes the CI failure where the test was validating that all CLI options are properly tested. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Description
When the
InlineModelResolverdeduplicates inline schemas that share the same structure, the Go code generator now creates type aliases using the original inline model names, pointing to the shared deduplicated type.Example
Given an OpenAPI spec with two different models that happen to have identically-structured inline schemas:
Before this PR:
The second inline schema would reuse the first deduplicated type, losing context:
After this PR:
Each field uses its own alias, preserving the original naming context:
Implementation Details
x-alias-namewhen deduplicating schemasCodegenProperty.dataTypeAlias(language-neutral)dataTypeanddataTypeAliasinpostProcessModels()so struct fields use the alias names[]RecipeIngredients)Benefits
[]RecipeIngredientsinstead of[]BlogPostAuthor)Testing
Added comprehensive test
typeAliasForDeduplicatedInlineSchemasTestcovering:Configuration Option
A configuration option
generateTypeAliasesForDedupedSchemashas been added (defaults totrue). Users who prefer the old behavior can disable this feature:This provides backward compatibility while enabling the improved naming by default.
Summary by cubic
Generates Go type aliases for deduplicated inline model schemas so fields keep their original, contextual names. Adds a
generateTypeAliasesForDedupedSchemasoption (default: true) and wires it into CLI, docs, and tests.InlineModelResolvertags deduped inline schemas withx-alias-name(respectsinlineSchemaNameMapping).DefaultCodegensetsCodegenProperty.dataTypeAlias; updates equality/hash/toString.type A = Binmodel_simple.mustache; gated bygenerateTypeAliasesForDedupedSchemasvia CLI/additional-properties.additionalProperties: true) schemas; direct$refdoes not create aliases.docs/generators/go.md); option exposed inGoClientCodegen; tests cover resolver alias tagging/mapping, direct properties, arrays, maps, the CLI option; sample updated.Written for commit f5db99c. Summary will update on new commits.